feat(fdc): Add execute_graphql signatures and integration test suite - #970
feat(fdc): Add execute_graphql signatures and integration test suite#970mk2023 wants to merge 11 commits into
Conversation
Implemented _make_gql_request on _DataConnectApiClient to execute and handle responses/errors for GraphQL operations. Added corresponding unit tests in tests/test_data_connect.py.
Refactored _parse_graphql_response and added robust recursive type deserialization to _DataConnectApiClient. - Implemented _deserialize_type and _deserialize_dataclass helper methods to support nested dataclasses, generic lists (List[T]), generic dictionaries (Dict[K, V]), Unions (Union[...]), Enums, and primitive casting. - Enhanced _make_gql_request error handling to prevent silent error swallowing when the errors key is present. - Added comprehensive unit test coverage in tests/test_data_connect.py.
…helper - Introduced QueryError subclass of FirebaseError for Data Connect GraphQL query/mutation errors and exposed it in __all__. - Extracted _check_graphql_errors helper method on _DataConnectApiClient. - Updated error handling for non-dictionary response payloads in _parse_graphql_response to raise InternalError. - Note: Did not edit parse_graphql_response because we are waiting on whether this will even be a function or not.
…nt instantiation - Removed output deserialization helpers (_extract_actual_type, _deserialize_type, _deserialize_dataclass) to return raw JSON payload dictionaries (ExecuteGraphqlResponse.data), aligning Data Connect with Firestore and Realtime Database patterns for user-defined schemas. - Updated DataConnect.__init__ to immediately instantiate _DataConnectApiClient for consistency with Node.js and other Python Admin SDK services. - Updated test suite in tests/test_data_connect.py to cover raw response parsing and immediate client instantiation.
Added execute_graphql and execute_graphql_read method signatures and docstrings to DataConnect and _DataConnectApiClient. Also introduced a comprehensive integration test suite in integration/test_data_connect.py translated from Node.js Admin SDK integration tests.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces execute_graphql and execute_graphql_read methods to the DataConnect client, along with corresponding integration and unit tests. The review feedback highlights that several of these new methods are left unimplemented (raising NotImplementedError) and provides their implementation details. Additionally, the reviewer identifies a critical type-checking bug in the validation logic when variables_type is Any, and points out a mismatch in a test assertion message.
stephenarosaj
left a comment
stephenarosaj
left a comment
There was a problem hiding this comment.
in progress, leaving comments early - have to look at test cases still
Added an explicit __init__ constructor to Impersonation to validate parameter configurations at object instantiation time. Enforced choosing either unauthenticated=True or auth_claims, with support for both auth_claims (snake_case) and authClaims (camelCase). Updated class docstring to recommend factory methods. Also added unit test suite TestImpersonation in tests/test_data_connect.py.
…mulator test setup Implemented execute_graphql and execute_graphql_read methods on DataConnect and _DataConnectApiClient. Configured Data Connect emulator schema, connector, queries, mutations, seed script, and GitHub Actions CI workflow step.
stephenarosaj
left a comment
stephenarosaj
left a comment
There was a problem hiding this comment.
LGTM with some changes requested - mostly small stuff, only a few blocking comments
stephenarosaj
left a comment
stephenarosaj
left a comment
There was a problem hiding this comment.
Accidentally selected Approve but mean Request changes - re-submitting review
LGTM with some changes requested - mostly small stuff, only a few blocking comments
…d streamlined read integration tests
| if not isinstance(auth_claims, dict): | ||
| raise ValueError("'auth_claims' must be a dictionary.") | ||
| super().__init__(authClaims=claims) | ||
| super().__init__(authClaims=auth_claims) |
There was a problem hiding this comment.
think there might still be a typo here - should be auth_claims=auth_claims right?
There was a problem hiding this comment.
I was also confused about this, but the snake_case auth_claims is used in the __init__ signature and the authenticated() factory method so the public API remains purely Pythonic. However, because this class inherits from dict, the super().__init__(authClaims=...) is required to translate that pythonic argument into the exact camelCase authClaims JSON key that the Firebase Data Connect API expects. If we change the super().__init__ call to snake_case, the backend will reject the payload.
There was a problem hiding this comment.
This is a great call out - I think we should remedy this, and it shouldn't be too complex. This is how I'm thinking of it:
authClaimsis what the backend API expects - this is how the data should be represented when it's turned into JSON and sent over the wireauth_claimsis the Pythonic, user-facing representation of that same data- If users were to print an instance of the
Impersonationclass at runtime, they would seeauthClaims- but this is the JSON representation, not the Python representation! We should instead be using theauth_claimsrepresentation here, since that's what users will interact with and it matches the platform via which they are interacting (Python) - We should keep the Python representation when in user-facing Python land, and use the JSON wire representation when communicating with the backend (instead of communicating with users) - this implies that we need some "translation" between the two
- Lucky for us, we already have a
_prepare_graphql_payloadmethod which "Serializes input query and options to JSON-compatible dictionary" ;)
- Lucky for us, we already have a
2 participants
Added
execute_graphqlandexecute_graphql_readmethod signatures and docstrings toDataConnectand_DataConnectApiClient. Expanded unit test coverage intests/test_data_connect.pyfor payload formatting, options validation, and error bubbling, and introduced a comprehensive integration test suite inintegration/test_data_connect.pytranslated from Node.js Admin SDK integration tests.